Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: drop Travis in favor of Actions #32450

Closed
wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

GitHub Actions is running all tests already present on Travis, as well
as building on more platforms (OS X and Windows). With Travis we're also
getting timeouts more frequently than with Actions, which gives the
false impression tests are failing (making it harder to triage PRs ready
to merge).

To make our config simpler, CI.yml and pythonpackage.yml got merged. The
coverage is also increased by running tests on OS X.

Signed-off-by: Matheus Marchini mmarchini@netflix.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. labels Mar 24, 2020
@mmarchini
Copy link
Contributor Author

Note that the two Travis jobs which were allowed to fail were removed (commit message lint and flake8 on dependencies) because I don't think folks were checking it (since GitHub reported those as success). But I might be wrong, so I'm happy to add them back if folks want to (commit message linting is already proposed on #32336).

@mmarchini
Copy link
Contributor Author

Alternatively, we might want to break this into multiple files (workflows), so if one of them fail due to flaky tests, we can rerun it separately (I think that's how it works, although I'm not sure).

@gengjiawen
Copy link
Member

I am thinking keep pythonpackage.yml as it is. Just remove travis config file.

@sam-github
Copy link
Contributor

Despite being a fan of good commit messages, I don't think anybody checks the builds if they passed, fine by me to delete it.

Copy link
Member

@himself65 himself65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove Travis LGTM. It will auto-canceled if running more than 45mins

GitHub Actions look better. but It a new CI so will also have other issues, example #31329

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly certain most of the lint jobs don't require Python.

There are references to Travis in our collaborator's guide which need to be removed.

I'd be in favour of keeping separate workflows as at the moment you can only restart a complete workflow and not an individual job. Maybe move some of the build jobs out of CI.yml (or move the various linters into something like lint.yml) but that could be done separately.

.github/workflows/CI.yml Outdated Show resolved Hide resolved
.github/workflows/CI.yml Outdated Show resolved Hide resolved
@mmarchini
Copy link
Contributor Author

I am thinking keep pythonpackage.yml as it is. Just remove travis config file.

I'm fine keeping pythonpackage.yml, but I would like to understand the reasoning first.

@mmarchini
Copy link
Contributor Author

I'd be in favour of keeping separate workflows as at the moment you can only restart a complete workflow and not an individual job. Maybe move some of the build jobs out of CI.yml (or move the various linters into something like lint.yml) but that could be done separately.

I like this idea. The reason I removed pythonpackage.yml is that it was re-building on Linux (while we also build on Linux on CI.yml) and it ran our JS tests (so it was not just a Python check, it was testing almost everything, which kills the purpose of a Python workflow). I'll re-work the actions as:

  • linters.yml
  • build-linux.yml
  • build-osx.yml
  • build-windows.yml
  • misc.yml (for building docs and other minor checks which doesn't qualify as linters)

@richardlau
Copy link
Member

@mmarchini I'd suggest build-macos.yml instead of build-osx.yml but otherwise the plan sounds good.

@mmarchini mmarchini added the wip Issues and PRs that are still a work in progress. label Mar 24, 2020
@gengjiawen
Copy link
Member

On the python part, not sure @cclauss want to do multi python version verify ?

@@ -0,0 +1,23 @@
name: test-macOS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since jenkins convers all platform test, maybe just test linux on github action is enough ?

Copy link
Contributor Author

@mmarchini mmarchini Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running on all three platforms gives authors an early signal on all Tier 1 platforms (I could even argue that we should be testing ARM here nevermind, ARM is for self-hosted hosts only, which is a whole other conversation). This is less a matter of coverage (we're not covering anything extra with Travis or GitHub Actions today), but rather a matter of early feedback on an easier to consume interface for outside contributors (this was the goal when Travis was introduced at least).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add macOS, should we add windows test too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yes. I just didn't add because I have no idea how we test Windows on Jenkins (the commands I used on Linux and macOS came from Jenkins config, so they are more suited for CI environments).

If anyone knows the best way to run tests on Windows on CI, feel free to comment :)

There's always the option to add tests for Windows in a follow up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with Tier 1 platforms for now. Linux is actually the slowest platform on Actions for our build/tests (* may possibly change if we start running tests on Windows).

I'm also happy for tests to be added on Windows in a follow up PR (we're not running tests on Windows in Actions or Travis CI before this PR).

@mmarchini
Copy link
Contributor Author

On the python part, not sure @cclauss want to do multi python version verify ?

There was no multi python version before, which is why I removed the file. And if we want to introduce multi-version, there's no need for a separate file (matrix is a job attribute, not a file attribute).

@cclauss
Copy link
Contributor

cclauss commented Mar 26, 2020

https://github.com/nodejs/node/blob/master/.github/workflows/pythonpackage.yml#L12 Is the way to turn on multi Python verify but my sense is that passing on Py 3.5 (until September) and 3.8 is sufficient? Py 2.7 is no longer tested? Is support for that dropped?
https://devguide.python.org/#status-of-python-branches

@mmarchini
Copy link
Contributor Author

https://github.com/nodejs/node/blob/master/.github/workflows/pythonpackage.yml#L12 Is the way to turn on multi Python

Right. Not sure what others think, but keeping a comment in the workflow file to "turn on" other versions is not very useful, since you'll need to edit the file and commit the changes on a PR to test it. The same workflow can be accomplished by just changing the version on the file, no need to keep a stray comment. Also, the comment doesn't mean those versions are supported, supported versions are listed on BUILDING.md.

my sense is that passing on Py 3.5 (until September) and 3.8 is sufficient

I'm fine running Py 3.5 and 3.8 on linters since those are fast, but doing so on the whole build seems too much (we'd be spawning 3 other runs for ~1 hour each). We are not doing this right now anyway, so it's out of the scope of this PR. If running both is really important, seems like something we should be doing on Jenkins instead. Or maybe only in one platform, but we'll be losing coverage in that case.

Py 2.7 is no longer tested? Is support for that dropped?

In theory it's tested on Jenkins, but I don't want to say it for sure.

@mmarchini
Copy link
Contributor Author

mmarchini commented Mar 26, 2020

If folks are unhappy with the changes I made to pythonpackage.yml, I can revert it and keep this file untouched. The goal of this PR is to drop Travis, and I don't want to diverge the focus to Python coverage in our build infrastructure, I only changed/removed that file because it was redundantly building on Linux.

GitHub Actions is running all tests already present on Travis, as well
as building on more platforms (OS X and Windows). With Travis we're also
getting timeouts more frequently than with Actions, which gives the
false impression tests are failing (making it harder to triage PRs ready
to merge).

To make our config simpler, CI.yml and pythonpackage.yml got merged. The
coverage is also increased by running tests on OS X.

Signed-off-by: Matheus Marchini <mmarchini@netflix.com>
@mmarchini
Copy link
Contributor Author

@richardlau I believe I addressed all your comments :)

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. I think we are fine to focus only on Python 3.8 for the first line testing.

Jenkins can deal with all the combinations and permutations of hardware and language versions.

Copy link
Member

@gengjiawen gengjiawen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mmarchini
Copy link
Contributor Author

mmarchini commented Mar 26, 2020

I don't think it deals with all combinations. Might be worth asking on nodejs/build. But I'm 99% sure we use Python 2.7 on some platforms and at least one 3.x on others.

@himself65
Copy link
Member

Github Actions haven't cache by default. Should we add actions/cache@v1 to the config?
It will take less time to complete.

@richardlau
Copy link
Member

Github Actions haven't cache by default. Should we add actions/cache@v1 to the config?
It will take less time to complete.

It's outside the scope of this PR. I've looked at caching but GitHub's implementation is very rudimentary and you have to tell it what to cache and restore. I've looked at implementing ccache (which Travis has built in support for) but we'd have to do a lot more work and there didn't seem to be an easy way to manage the cache (I've had to manually delete the cache several times for Travis to get passing runs).

There's also a limits for caches:

GitHub will remove any cache entries that have not been accessed in over 7 days. There is no limit on the number of caches you can store, but the total size of all caches in a repository is limited to 5 GB. If you exceed this limit, GitHub will save your cache but will begin evicting caches until the total size is less than 5 GB.

@richardlau
Copy link
Member

I don't think it deals with all combinations. Might be worth asking on nodejs/build. But I'm 99% sure we use Python 2.7 on some platforms and at least one 3.x on others.

We do not have the capability (either people wise or resources) to test every combination of build tooling. All of the hardware used by build has been generously donated and we need to be able to build two LTS lines, current and master which all have different spreads of minimum supported toolchain versions. With regards to Python, there was an effort last year to get Python 3 onto as many of the CI hosts as possible. Newer hosts (e.g. the macOS 10.15 machines) will be Python 3.

@mmarchini mmarchini added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Mar 26, 2020
@mmarchini
Copy link
Contributor Author

I believe this is a significant enough change to justify a @nodejs/collaborators ping. I'll wait a few days, if there are no objections I'll land it.

@mmarchini mmarchini self-assigned this Mar 26, 2020
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been gradually switching other open-source like yargs over to actions, and we've been using it at work for the past couple months too. I think this is a great move.

@addaleax
Copy link
Member

Landed in 7c66f45

addaleax pushed a commit that referenced this pull request Mar 27, 2020
GitHub Actions is running all tests already present on Travis, as well
as building on more platforms (OS X and Windows). With Travis we're also
getting timeouts more frequently than with Actions, which gives the
false impression tests are failing (making it harder to triage PRs ready
to merge).

To make our config simpler, CI.yml and pythonpackage.yml got merged. The
coverage is also increased by running tests on OS X.

Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

PR-URL: #32450
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@addaleax addaleax closed this Mar 27, 2020
addaleax pushed a commit that referenced this pull request Mar 30, 2020
GitHub Actions is running all tests already present on Travis, as well
as building on more platforms (OS X and Windows). With Travis we're also
getting timeouts more frequently than with Actions, which gives the
false impression tests are failing (making it harder to triage PRs ready
to merge).

To make our config simpler, CI.yml and pythonpackage.yml got merged. The
coverage is also increased by running tests on OS X.

Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

PR-URL: #32450
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 2, 2020
GitHub Actions is running all tests already present on Travis, as well
as building on more platforms (OS X and Windows). With Travis we're also
getting timeouts more frequently than with Actions, which gives the
false impression tests are failing (making it harder to triage PRs ready
to merge).

To make our config simpler, CI.yml and pythonpackage.yml got merged. The
coverage is also increased by running tests on OS X.

Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

PR-URL: nodejs#32450
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 2, 2020
GitHub Actions is running all tests already present on Travis, as well
as building on more platforms (OS X and Windows). With Travis we're also
getting timeouts more frequently than with Actions, which gives the
false impression tests are failing (making it harder to triage PRs ready
to merge).

To make our config simpler, CI.yml and pythonpackage.yml got merged. The
coverage is also increased by running tests on OS X.

Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

Backport-PR-URL: #32608
PR-URL: #32450
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@codebytere codebytere mentioned this pull request Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.